Date: Thu, 12 Feb 2015 18:56:51 +0000 From: Sreekanth Rupavatharam <rupavath@juniper.net> To: Jack Vogel <jfvogel@gmail.com> Cc: "jfv@freebsd.org" <jfv@freebsd.org>, "freebsd-net@freebsd.org" <freebsd-net@freebsd.org>, hiren panchasara <hiren@strugglingcoder.info> Subject: Re: Double cleanup in igb_attach Message-ID: <D102367E.1B663%rupavath@juniper.net> In-Reply-To: <CAFOYbcmPF70yf%2B7BmTdDrrfUtyPP7pkCx8_%2BQdZcfOSA_dAK1w@mail.gmail.com> References: <D0EC151C.1A7B1%rupavath@juniper.net> <20150127192814.GA63990@strugglingcoder.info> <D0ED2984.1A86C%rupavath@juniper.net> <CAFOYbcmBks6s448stkVjGsWxxNZdZEwX2zxtdUh5cDnmtKEA1w@mail.gmail.com> <26266AD2-4743-4A7B-A87D-F68E2E2425A0@juniper.net> <CAFOYbckAP6t0jkhtswi%2BRnFGtJtN7=feCw6_vneS%2BSEZn4o2Pg@mail.gmail.com> <BABD0D32-30E3-4521-8838-976FF77AE244@juniper.net> <CAFOYbcmPF70yf%2B7BmTdDrrfUtyPP7pkCx8_%2BQdZcfOSA_dAK1w@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Jack,
Actually, looking at the code again, it seems to me that igb_detach is not =
supposed to be called from igb_attach at all. It causes more problems than =
previously mentioned. E.g., in case of branching to err_late *before* igb_=
setup_interface(where the ifp is initialized), calling igb_detach there cau=
ses a NULL pointer access. The best way to deal with it is to take out the =
igb_detach call from the err_late: label. Thoughts? Comments?
-- Thanks,
Sreekanth
From: Jack Vogel <jfvogel@gmail.com<mailto:jfvogel@gmail.com>>
Date: Tuesday, January 27, 2015 at 12:42 PM
To: Sreekanth Rupavatharam <rupavath@juniper.net<mailto:rupavath@juniper.ne=
t>>
Cc: hiren panchasara <hiren@strugglingcoder.info<mailto:hiren@strugglingcod=
er.info>>, "freebsd-net@freebsd.org<mailto:freebsd-net@freebsd.org>" <freeb=
sd-net@freebsd.org<mailto:freebsd-net@freebsd.org>>, "jfv@freebsd.org<mailt=
o:jfv@freebsd.org>" <jfv@freebsd.org<mailto:jfv@freebsd.org>>
Subject: Re: Double cleanup in igb_attach
Yes, I will look them over.
Jack
On Tue, Jan 27, 2015 at 12:38 PM, Sreekanth Rupavatharam <rupavath@juniper.=
net<mailto:rupavath@juniper.net>> wrote:
Thanks jack,
Now, can you please review these changes? And commit if you deem it fi=
t?
Thanks,
-Sreekanth
On Jan 27, 2015, at 12:24 PM, "Jack Vogel" <jfvogel@gmail.com<mailto:jfvoge=
l@gmail.com>> wrote:
Errrr, I am one of those people :) (jack.vogel@intel.com<mailto:jack.vogel=
@intel.com>)
Jack
On Tue, Jan 27, 2015 at 12:21 PM, Sreekanth Rupavatharam <rupavath@juniper.=
net<mailto:rupavath@juniper.net>> wrote:
Definitely, but I didn't have the contact info of those people.
Thanks,
-Sreekanth
On Jan 27, 2015, at 12:15 PM, "Jack Vogel" <jfvogel@gmail.com<mailto:jfvoge=
l@gmail.com>> wrote:
If you want something committed to an Intel driver, asking Intel might be t=
he
courteous thing to do, don't you think?
Jack
On Tue, Jan 27, 2015 at 11:51 AM, Sreekanth Rupavatharam <rupavath@juniper.=
net<mailto:rupavath@juniper.net>> wrote:
Hiren,
Can you help commit this?
Index: if_igb.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- if_igb.c(revision 298053)
+++ if_igb.c(working copy)
@@ -723,7 +723,8 @@ igb_attach(device_t dev)
return (0);
err_late:
-igb_detach(dev);
+if(igb_detach(dev) =3D=3D 0) /* igb_detach did the cleanup */
+return(error);
igb_free_transmit_structures(adapter);
igb_free_receive_structures(adapter);
igb_release_hw_control(adapter);
-- Thanks,
Sreekanth
On 1/27/15, 11:28 AM, "hiren panchasara" <hiren@strugglingcoder.info<mailto=
:hiren@strugglingcoder.info>> wrote:
+ Jack
On Tue, Jan 27, 2015 at 12:00:19AM +0000, Sreekanth Rupavatharam wrote:
Apologies if this is not the right forum. In igb_attach function, we have t=
his code.
err_late:
igb_detach(dev);
igb_free_transmit_structures(adapter);
igb_free_receive_structures(adapter);
igb_release_hw_control(adapter);
err_pci:
igb_free_pci_resources(adapter);
if (adapter->ifp !=3D NULL)
if_free(adapter->ifp);
free(adapter->mta, M_DEVBUF);
IGB_CORE_LOCK_DESTROY(adapter);
The problem is that igb_detach also does the same cleanup in it?s body. Onl=
y exception is this case where it just returns EBUSY
/* Make sure VLANS are not using driver */
if (if_vlantrunkinuse(ifp)) {
device_printf(dev,"Vlan in use, detach first\n");
return (EBUSY);
}
I think the code in igb_attach should be changed to free up resources only =
if the igb_detach returns an error. Here?s the patch for it.
Index: if_igb.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- if_igb.c (revision 298025)
+++ if_igb.c (working copy)
@@ -723,7 +723,8 @@ igb_attach(device_t dev)
return (0);
err_late:
- igb_detach(dev);
+ if(igb_detach(dev) =3D=3D 0) /* igb_detach did the cleanup */
+ return;
igb_free_transmit_structures(adapter);
Can anyone comment on it and tell me if my understanding is incorrect?
Seems reasonable to me at the first glance.
We need to call IGB_CORE_LOCK_DESTROY(adapter) before returning though.
cheers,
Hiren
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?D102367E.1B663%rupavath>